interface/src/routes/AgentSkills.tsx (2)

234-240: Core composite-key logic is correct; document the known limitation of the name-only fallback.

The key shapes match correctly:

• New installs: installedKeys entry = "${source_repo}/${name}" (e.g. "anthropics/skills/weather"); compositeKey = "${skill.source}/${skill.name}" → same shape → correct match.
• Different source, same name: keys differ → no false positive → fixes #190 ✓.
• Legacy installs (no source_repo): installedKeys entry = "weather" → the fallback installedKeys.has(skill.name.toLowerCase()) marks all registry skills with that name as installed, which is the original bug behaviour.

This is intentional per the PR description, but it is worth a brief inline comment so future contributors understand why the fallback exists and the trade-off it carries:

📝 Suggested documentation comment

+	// installedKeys uses source_repo/name when available (new installs) for
+	// per-repo precision. Skills installed before this change have no source_repo
+	// and fall back to name-only matching, which re-exhibits the original false-
+	// positive behaviour until those skills are re-installed.
 	const installedKeys = new Set(
 		installedSkills.map((s) =>
 			s.source_repo
 				? `${s.source_repo}/${s.name}`.toLowerCase()
 				: s.name.toLowerCase(),
 		),
 	);

Also applies to: 371-374

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@interface/src/routes/AgentSkills.tsx` around lines 234 - 240, Add a brief
inline comment next to the installedKeys construction and the fallback
membership check explaining that legacy installs (installedSkills entries
without source_repo) are stored as name-only (e.g., "weather"), and the fallback
installedKeys.has(skill.name.toLowerCase()) will therefore mark any registry
skill with the same name as installed; state this is an intentional trade-off to
preserve legacy behavior and reference the compositeKey
(`${skill.source}/${skill.name}`) versus name-only key formats so future
contributors understand the limitation and why the fallback exists.

---

506-517: key={skill.name} on InstalledSkill is safe today but fragile.

SkillSet already enforces one-skill-per-lowercase-name, so duplicate keys cannot occur now. However, if that invariant ever relaxes (e.g. a future multi-source install mode), React will silently render only the last matching child.

Consider key={skill.source_repo ? \${skill.source_repo}/${skill.name}` : skill.name}` to make the key stable and unique under any future extension.

✏️ Proposed change

-								key={skill.name}
+								key={skill.source_repo ? `${skill.source_repo}/${skill.name}` : skill.name}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@interface/src/routes/AgentSkills.tsx` around lines 506 - 517, The current
InstalledSkill list uses key={skill.name} which relies on a fragile uniqueness
invariant; change the key generation in the installedSkills.map to a stable
unique composite (e.g., include skill.source_repo when present) so keys remain
unique if multiple skills share a name. Update the InstalledSkill mapping to
compute the key as something like `${skill.source_repo}/${skill.name}` when
skill.source_repo exists, otherwise fallback to skill.name, ensuring
InstalledSkill receives that composite key and preventing React child key
collisions in future multi-source installs.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/skills/installer.rs (2)

382-438: Good test coverage for inject_source_repo — edge cases and roundtrip with parse_frontmatter are well covered.

One gap worth noting: there's no test for content with leading whitespace before the frontmatter delimiter (e.g. " ---\nname: x\n---\n"). trim_start() on line 224 handles this, but a test would document the behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/skills/installer.rs` around lines 382 - 438, Add a new unit test that
verifies inject_source_repo correctly handles leading whitespace before the
frontmatter delimiter: create content with leading spaces before the initial
"---" (e.g. "  ---\nname: x\n---\n\n# Body\n"), call inject_source_repo(content,
"owner/repo"), and assert the returned string contains a single source_repo:
owner/repo inside the frontmatter (use parse_frontmatter or string checks),
preserves the existing fields (like name: x) and retains the body "# Body".
Reference inject_source_repo and parse_frontmatter to locate where the test
should sit alongside the other inject_source_repo tests.

---

222-259: inject_source_repo: minor leading-blank-line artifact in rewritten frontmatter.

When fm_block starts with \n (which it always does, since after_opening begins right after the opening ---), the first element from .lines() is "". After join("\n"), new_fm starts with an empty line, so the output looks like:

---
                          ← blank line here
name: weather
source_repo: anthropics/skills
---

This is functionally harmless — parse_frontmatter trims and skips blank lines — but it's a minor aesthetic wart. Consider stripping the leading newline from fm_block before splitting:

♻️ Optional cleanup

-    let fm_block = &after_opening[..end_pos];
+    let fm_block = after_opening[..end_pos].trim_start_matches('\n');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/skills/installer.rs` around lines 222 - 259, The inject_source_repo
function produces a leading blank line in the rewritten frontmatter because
fm_block begins with a newline; fix it by normalizing fm_block before splitting
(e.g., remove a leading '\n' or use trim_start_matches on fm_block) so that
.lines() doesn't yield an empty first element, then proceed with the existing
filtering/joining logic to append the new source_repo line.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro